-
Notifications
You must be signed in to change notification settings - Fork 173
[TF-28671] Add HYOK configuration resources #1841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cbe2506
to
944abc1
Compare
|
||
AgentPoolID types.String `tfsdk:"agent_pool_id"` | ||
Organization types.String `tfsdk:"organization"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reasoning behind using AWSOIDCConfigurationID
, GCPOIDCConfigurationID
, VaultOIDCConfigurationID
, and AzureOIDCConfigurationID
instead of
oidc_configuration_id
and oidc_configuration_type
like the HyokConfigurations model and database schema uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also a valid way to approach it. At the end of the day, it just needs to populate the tfe.OIDCConfigurationTypeChoice
object that looks like:
type OIDCConfigurationTypeChoice struct {
AWSOIDCConfiguration *AWSOIDCConfiguration
GCPOIDCConfiguration *GCPOIDCConfiguration
AzureOIDCConfiguration *AzureOIDCConfiguration
VaultOIDCConfiguration *VaultOIDCConfiguration
}
I don't really have strong feelings about this, but I think having an explicit type could shorten the code a little.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this works well, I was just curious if there was a technical reason or constraint. Thanks!
41375cb
to
99097c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested locally, good stuff!
tested for AWS config. I was able to plan, apply and destroy (after revoking the key):
|
gcp test:
|
azure test:
|
vault test:
|
KMSOptions: kmsOptions, | ||
} | ||
|
||
if p.OIDCConfiguration.AWSOIDCConfiguration != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since were doing a switch-case on the top of the file, I'd suggest we do a switch-case here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I am not really sure how a switch statement would work here. Each p.OIDCConfiguration
is of type OIDCConfigurationTypeChoice
:
type OIDCConfigurationTypeChoice struct {
AWSOIDCConfiguration *AWSOIDCConfiguration
GCPOIDCConfiguration *GCPOIDCConfiguration
AzureOIDCConfiguration *AzureOIDCConfiguration
VaultOIDCConfiguration *VaultOIDCConfiguration
}
So one of these pointers will be non-nil.
Because of this, I'm not sure which expression to put as the switch, unlike the stuff that uses switch statements above, switching on m.OIDCConfigurationType.ValueString()
- which can be either "aws", "vault", "gcp" or "azure".
I might be blanking here, so if you think of how to use a switch statement, I'll be more than happy to make those changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what I mean:
switch {
case p.OIDCConfiguration.AWSOIDCConfiguration != nil:
model.OIDCConfigurationID = types.StringValue(p.OIDCConfiguration.AWSOIDCConfiguration.ID)
model.OIDCConfigurationType = types.StringValue(OIDCConfigurationTypeAWS)
case p.OIDCConfiguration.GCPOIDCConfiguration != nil:
model.OIDCConfigurationID = types.StringValue(p.OIDCConfiguration.GCPOIDCConfiguration.ID)
model.OIDCConfigurationType = types.StringValue(OIDCConfigurationTypeGCP)
case p.OIDCConfiguration.AzureOIDCConfiguration != nil:
model.OIDCConfigurationID = types.StringValue(p.OIDCConfiguration.AzureOIDCConfiguration.ID)
model.OIDCConfigurationType = types.StringValue(OIDCConfigurationTypeAzure)
case p.OIDCConfiguration.VaultOIDCConfiguration != nil:
model.OIDCConfigurationID = types.StringValue(p.OIDCConfiguration.VaultOIDCConfiguration.ID)
model.OIDCConfigurationType = types.StringValue(OIDCConfigurationTypeVault)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested and this worked locally for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I applied them
6dbdfa5
to
ca44c18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, I agree with @iuri-slywitch-hashicorp on his preference for a switch statement in modelFromTFEHYOKConfiguration
but it's non-blocking for me
resource_tfe_aws_oidc_configuration.go and basic test resource_tfe_gcp_oidc_configuration.go and test resource_tfe_azure_oidc_configuration.go_oidc_configuration.go and test resource_tfe_vault_oidc_configuration.go and tests Add HYOK_ORGANIATION_NAME environment variable to testing.md Add documentation for resources Add to CHANGELOG.md update basic usage in vault_oidc_configuration.html.markdown Do not require replace for everything Update documentation and default value of auth_path skipIfEnterprise update pricing info Implement resource_tfe_hyok_configuration.go Add hyok_configuration.html.markdown fix bug where aws configs recognized as gcp configs Add in acceptance tests Wait for revoked status during acceptance test Add CHANGELOG.md
a875389
to
b28648a
Compare
Description
Describe why you're making this change.
Remember to:
Testing plan
terraform plan
andterraform apply
terraform destroy
on these resources to make sure they can be deletedExternal links
Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.
Output from acceptance tests
Please run applicable acceptance tests locally and include the output here. See testing.md to learn how to run acceptance tests.
If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.
Rollback Plan
Changes to Security Controls